Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: head-support race condition #2599 #2600

Closed
wants to merge 1 commit into from
Closed

fix: head-support race condition #2599 #2600

wants to merge 1 commit into from

Conversation

stukennedy
Copy link

@stukennedy stukennedy commented Jun 7, 2024 •
edited
Loading

Description

currently the head-support feature starts doing its merging based off an htmx:afterSwap event. This means that any script being loaded into the DOM from the htmxSwap that wants to run some initialisation code may run after the htmx:afterHeadMerge event has triggered, and never see it.

An example for this would be if you want to return a ChartJS from an endpoint along with the script to display the chart.

    <head>
      <script src="https://cdn.jsdelivr.net/npm/chart.js"></script>
    </head>
    <canvas id="myChart"></canvas>
    <script>
      htmx.on('htmx:afterHeadMerge', function () {
        (async () => {
          while (!Chart) {
            await new Promise((resolve) => setTimeout(resolve, 100));
          }
          var ctx = document.getElementById('myChart').getContext('2d');
          var myChart = new Chart(ctx, { <CONFIG> });
        })();
      });
    </script>

The polling is necessary to check that the Chart class has been declared (i.e. the new head script has loaded)
(and yes this could do with a timeout so it doesn't try for ever, but this is the basic solution)
However, this code won't work every time if we have htmx:afterSwap in head-support.js as the trigger.

Changing htmx:afterSwap to htmx:afterSettle ensures the DOM has settled (i.e. our event listener has been registered) before the head swap functionality is performed and our code works as expected.

Testing

This was tested with the above code by calling this code on a lazy-loaded hx-post.
The race condition seems to be gone.

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

@Telroshan
Copy link
Collaborator

Hello, please note that for htmx 2 (aka the dev branch here), we moved extensions to a separate repository.
So you'll want to either retarget this PR to the v1 branch, in the extension's src directory, or port this PR to the new extensions repo (you may want to check that htmx 2 + the v2 extension doesn't already solve this btw)

@Telroshan Telroshan added bug Something isn't working extension Consideration for an extension labels Jun 8, 2024
@1cg
Copy link
Contributor

1cg commented Jun 27, 2024

Yep, please retarget against v1 and the new extensions repo. Thank you!

@1cg
Copy link
Contributor

1cg commented Jul 8, 2024

Hey stu checking in here, can you retarget the v1 branch and port the change over to the new extensions repo?

@stukennedy stukennedy changed the base branch from dev to v1 July 9, 2024 07:01
@stukennedy stukennedy changed the base branch from v1 to dev July 9, 2024 07:13
@stukennedy
Copy link
Author

stukennedy commented Jul 9, 2024 •
edited
Loading

Hey stu checking in here, can you retarget the v1 branch and port the change over to the new extensions repo?

Apologies, totally forgot to see this through.

Opened a new PR on the other branch:
#2704

And also in the new htmx-extensions repo:
bigskysoftware/htmx-extensions#45

@stukennedy stukennedy closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working extension Consideration for an extension
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stukennedy @Telroshan @1cg